fix(osint): enforce risk-envelope existence and envelope-driven alert budgets in policies#19506
Conversation
BrianCLong
added
the
codex
ChatGPT Codex Connector
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request establishes a foundational governance framework for OSINT collection by integrating risk envelopes directly into policy evaluation. It ensures that collection activities adhere to predefined constraints regarding allowed methods, alert budgets, data privacy, and provenance. The changes introduce a deny-by-default approach, supported by new configuration files, JSON schemas, and automated CI checks, to provide a robust and verifiable system for managing OSINT data. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Ignored Files
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (46)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive set of OPA policies, configurations, schemas, and tests for OSINT governance. The changes are well-structured and align with the goal of creating a robust, testable, and deny-by-default system. However, a critical 'fail-open' vulnerability has been identified: several individual rules fail to account for missing input fields. This allows an attacker to bypass security checks (e.g., alert budgets, provenance requirements, and method allowlists) by omitting required fields, as OPA comparisons with undefined variables cause rules not to fire. Explicit existence checks (e.g., not input.field) should be added to reject malformed or incomplete inputs. Additionally, there are suggestions to improve the conciseness and maintainability of the new Rego policies.
| deny["alert_budget_exceeded"] if { | ||
| envelope := input.risk_envelopes[input.risk_envelope] | ||
| input.collection.alert_count > envelope.max_alerts_per_run | ||
| } |
There was a problem hiding this comment.
The alert_budget_exceeded rule fails to deny if input.collection.alert_count is missing. In OPA, if a variable used in a comparison is undefined, the entire rule is undefined and does not fire. Since the allow rule (lines 14-16) only checks if there are zero deny results, omitting this field effectively bypasses the alert budget enforcement. You should add an explicit check for the presence of alert_count.
| deny["missing_artifacts"] if { | ||
| count(input.collection.provenance.artifact_ids) == 0 | ||
| } | ||
|
|
||
| deny["escalation_single_source"] if { | ||
| input.collection.provenance.escalation | ||
| input.collection.provenance.single_source | ||
| } | ||
|
|
||
| deny["insufficient_corroboration"] if { | ||
| input.collection.provenance.escalation | ||
| input.collection.provenance.corroboration_count < 2 | ||
| } |
There was a problem hiding this comment.
The provenance policy fails to deny if the input.collection.provenance object is missing. All deny rules in this package depend on fields within input.collection.provenance. If the object is missing, none of the deny rules will fire, and the allow rule (lines 19-21) will evaluate to true. This allows bypassing all provenance requirements (artifact IDs, corroboration counts, etc.) by simply omitting the provenance data.
| deny["method_not_allowed"] if { | ||
| source := input.source_registry[input.collection.source_id] | ||
| method := input.collection.method | ||
| not method_allowed(source.allowed_methods, method) | ||
| } | ||
|
|
||
| deny["method_not_allowed_by_envelope"] if { | ||
| envelope := input.risk_envelopes[input.risk_envelope] | ||
| method := input.collection.method | ||
| not method_allowed(envelope.allowed_methods, method) | ||
| } |
There was a problem hiding this comment.
The ToS policy fails to deny if input.collection.method is missing. The rules method_not_allowed (lines 13-17) and method_not_allowed_by_envelope (lines 19-23) will not fire if method is undefined. If the source and envelope are otherwise valid, the policy will allow the request, bypassing the method allowlist checks. You should add an explicit check to ensure the collection method is specified.
| deny["retention_ttl_missing"] if { | ||
| not input.collection.retention_ttl_days | ||
| } | ||
|
|
||
| deny["retention_ttl_missing"] if { | ||
| input.collection.retention_ttl_days <= 0 | ||
| } |
There was a problem hiding this comment.
The two deny rules for checking retention_ttl_days can be combined into a single, more concise rule. This improves readability and maintainability. Also, the key retention_ttl_missing is not fully descriptive for the _ <= 0 case. A key like retention_ttl_invalid would be more accurate.
This single rule correctly handles cases where retention_ttl_days is missing, null, zero, or negative.
deny["retention_ttl_invalid"] if {
not input.collection.retention_ttl_days > 0
}
| deny["disallowed_tos_method"] if { | ||
| input.collection.method == "circumvent" | ||
| } | ||
|
|
||
| deny["disallowed_tos_method"] if { | ||
| input.collection.method == "credential_share" | ||
| } | ||
|
|
||
| deny["disallowed_tos_method"] if { | ||
| input.collection.method == "paywall_bypass" | ||
| } |
There was a problem hiding this comment.
The multiple deny["disallowed_tos_method"] rules for hardcoded method names can be consolidated. Using a set of disallowed methods makes the policy more readable and easier to maintain if more methods need to be added in the future.
disallowed_methods := {"circumvent", "credential_share", "paywall_bypass"}
deny["disallowed_tos_method"] if {
disallowed_methods[input.collection.method]
}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Temporarily closing to reduce Actions queue saturation and unblock #22241. Reopen after the golden-main convergence PR merges. |
1 similar comment
|
Temporarily closing to reduce Actions queue saturation and unblock #22241. Reopen after the golden-main convergence PR merges. |
1 participant
Motivation
Description
allowed_methodschecks intos.regoand envelope-driven alert budgets inalert_budget.rego.tos,alert_budget,privacy, andprovenance, plus updated fixtures to includerisk_envelopesobjects for deterministic testing.config/osint/(sources.yml,risk_envelopes.yml,retention.yml) and JSON schemas underschemas/osint/for source registry, risk envelopes, provenance receipts, collection events, and redaction reports..github/workflows/osint-governance-verify.ymlto install OPA and runopa testplus schema presence checks, and added evidence artifacts and an agent example and prompt registry entries to document scope and verification requirements.Testing
node scripts/check-boundaries.cjslocally and it completed with no boundary violations.opa test .github/policies/osintbutopawas not available in the execution environment (the workflow is configured to runopa testin CI via theopen-policy-agent/setup-opaaction).make smoke(bootstrap) but the local bootstrap failed due to a pip/proxy environment error that prevented dependency installation, so full smoke tests could not complete locally; CI will execute the OPA test suite and the schema presence checks added to the workflow.Codex Task